-
-
Notifications
You must be signed in to change notification settings - Fork 954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Notification age (x minutes ago) on notifcation screen #835
Notification age (x minutes ago) on notifcation screen #835
Conversation
Great feature. |
Works nicely. Only problem is that the 'no notifications' notification has it (and it doesn't seem to update). |
Not sure if this is still active, but this would be great to get merged. What is left to do / blocking this from getting in? |
The problem seems to have been me forgetting about it. Now I think this has to be adjusted to newer changes that happened in the meantime. I don't have time today but I can try doing that tomorrow. |
Friendly bump 🙏 |
@clemensvonmolo @devnoname120 Sorry for not providing any feedback about this PR for so long. Is there any reason for it to be still a draft? |
I can set it to a normal PR for now, but Github says there will be conflicts and I can't do anything about that in the next three days while I'm on a class trip. It might also be a good idea to discuss the positioning of the text again. Anyway thanks for the positive feedback and patience everyone! |
Okay this should work now. I've also created InfiniTimeOrg/InfiniSim#46 to fix the build issue it is currently facing with this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow up PR there could be special messages like "Just now" or "Yesterday".
} else { | ||
timeUnit = 'm'; | ||
} | ||
lv_obj_t* alert_age = lv_label_create(container, nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables names should be like this. Don't commit this suggestion.
lv_obj_t* alert_age = lv_label_create(container, nullptr); | |
lv_obj_t* alertAge = lv_label_create(container, nullptr); |
lv_obj_t* alert_age = lv_label_create(container, nullptr); | ||
lv_label_set_text_fmt(alert_age, "%d%c ago", ageInt, timeUnit); | ||
// same format as alert_count | ||
lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, 0, -16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make the padding similar to the text.
lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, 0, -16); | |
lv_obj_align(alert_age, container, LV_ALIGN_IN_BOTTOM_RIGHT, -10, -10); |
if ((ageInt / (60 * 24)) >= 1) { | ||
ageInt /= (60 * 24); | ||
timeUnit = 'd'; | ||
} else if ((ageInt / 60) >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be simplified.
if ((ageInt / (60 * 24)) >= 1) { | |
ageInt /= (60 * 24); | |
timeUnit = 'd'; | |
} else if ((ageInt / 60) >= 1) { | |
if (ageInt >= 60 * 24) { | |
ageInt /= (60 * 24); | |
timeUnit = 'd'; | |
} else if (ageInt >= 60) { |
@@ -57,6 +62,7 @@ namespace Pinetime { | |||
size_t NbNotifications() const; | |||
|
|||
private: | |||
Controllers::DateTime& dateTimeController; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can even store a const
reference, to make it explicit, that a notification can't change the date-time, just get the current date and time
Controllers::DateTime& dateTimeController; | |
const Controllers::DateTime& dateTimeController; |
std::array<char, MessageSize + 1> message; | ||
Categories category = Categories::Unknown; | ||
|
||
const char* Message() const; | ||
const char* Title() const; | ||
}; | ||
|
||
NotificationManager(Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can even store a const
reference, to make it explicit, that a notification can't change the date-time, just get the current date and time
NotificationManager(Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} { | |
NotificationManager(const Controllers::DateTime& dateTimeController) : dateTimeController {dateTimeController} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At present it is not possible since DateTime controller updates it member variables during time retrieving.
InfiniTime/src/components/datetime/DateTimeController.cpp
Lines 72 to 102 in d69cfcf
std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds> DateTime::CurrentDateTime() { | |
xSemaphoreTake(mutex, portMAX_DELAY); | |
UpdateTime(nrf_rtc_counter_get(portNRF_RTC_REG), false); | |
xSemaphoreGive(mutex); | |
return currentDateTime; | |
} | |
void DateTime::UpdateTime(uint32_t systickCounter, bool forceUpdate) { | |
// Handle systick counter overflow | |
uint32_t systickDelta = 0; | |
if (systickCounter < previousSystickCounter) { | |
systickDelta = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - previousSystickCounter; | |
systickDelta += systickCounter + 1; | |
} else { | |
systickDelta = systickCounter - previousSystickCounter; | |
} | |
auto correctedDelta = systickDelta / configTICK_RATE_HZ; | |
// If a second hasn't passed, there is nothing to do | |
// If the time has been changed, set forceUpdate to trigger internal state updates | |
if (correctedDelta == 0 && !forceUpdate) { | |
return; | |
} | |
auto rest = systickDelta % configTICK_RATE_HZ; | |
if (systickCounter >= rest) { | |
previousSystickCounter = systickCounter - rest; | |
} else { | |
previousSystickCounter = static_cast<uint32_t>(portNRF_RTC_MAXTICKS) - (rest - systickCounter - 1); | |
} | |
currentDateTime += std::chrono::seconds(correctedDelta); |
It can be overcomed with mutable
but I am not sure whether this is a good idea.
Any plans to rebase/eventually merge this sometime? Seems it would be a nice improvement. |
This would go very well with #1697 if the grey notification box was made slightly smaller vertically to allow a small bottom bar for this and the notification icons. |
lv_obj_t* alert_age should be the last object to be created. Otherwise, objects such as alert_caller will be on top of it and cause the alert_age to be invisible in IncomingCall message. This also needs a rebase. |
Yet another friendly bump :) |
It looks like the original author is not working on this anymore, so if anyone would like to rebase this + apply the feedback onto a new PR you'd be totally welcome to do that |
Rebased on the latest |
Feel free to continue. I wouldn't have had time until after Christmas anyways and on top of that I'm not currently up to date with the codebase of InfiniTime which would only mean more work for me. |
This PR adds "x time ago" to the bottom of the notification screen. Every notification item now stores the time at which it was received as a std::time_t (saw no reason to use std::chrono::time_point as it takes more memory with no advantages for storage). The time arrived is set in the notification manager so existing apps don't have to change anything.
I'm not really sure about the location of the text (image below) and if it would be better to use C-style string operations for memory usage and allocation optimisation.
..e indicator does not interfere with the message